8343336: Add persistentScrollBars preference #1618

mstr2 · 2024-10-31T03:56:30Z

This PR adds another accessibility preference that is available on all supported desktop platforms:

Windows: Settings -> Accessibility -> Visual Effects -> Always show scroll bars
macOS: System Settings -> Appearance -> Show scroll bars
Ubuntu: Settings -> Accessibility -> Seeing -> Always show scroll bars

When the OS setting is changed, all platforms report updates that can be observed in JavaFX.
The manual test application test/manual/events/PlatformPreferencesChangedTest can be used to test the feature.

/reviewers 2
/csr

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8343451 to be approved
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8343336: Add persistentScrollBars preference (Enhancement - P4)
JDK-8343451: Add persistentScrollBars preference (CSR)

Reviewers

Andy Goryachev (@andy-goryachev-oracle - Reviewer)
Kevin Rushforth (@kevinrushforth - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1618/head:pull/1618
$ git checkout pull/1618

Update a local copy of the PR:
$ git checkout pull/1618
$ git pull https://git.openjdk.org/jfx.git pull/1618/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1618

View PR using the GUI difftool:
$ git pr show -t 1618

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1618.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-31T03:57:11Z

👋 Welcome back mstrauss! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-10-31T03:57:22Z

@mstr2 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8343336: Add persistentScrollBars preference

Reviewed-by: angorya, kcr

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 5 new commits pushed to the master branch:

03eb8b1: 8344899: Map RT-nnnn bug IDs to JDK-mmmmmmm in JavaFX sources
bdfc338: 8338000: [macos] Remove GlassFullscreenWindow
a45e33c: 8342461: Remove calls to doPrivileged in javafx.web/{android,ios}
f7566ad: 8345063: Centralize version number of the Gradle build file
4af67c1: 8344906: Simplify Java version parsing in the build file

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-10-31T03:57:55Z

@mstr2
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

openjdk · 2024-10-31T03:58:17Z

@mstr2 has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@mstr2 please create a CSR request for issue JDK-8343336 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

mlbridge · 2024-10-31T04:02:26Z

Webrevs

andy-goryachev-oracle · 2024-10-31T17:56:07Z

This looks like a useful data point.

I am curious as to what are the criteria for including or excluding a particular preference? What is our long term goal - should we support all the preferences available on a particular platform, or the intersection, or what? May be it's something to discuss first on the mailing list.

mstr2 · 2024-10-31T18:16:51Z

My thinking is that a particular preference should be useful and almost universally supported on the desktop platforms to make it into JavaFX API. The number of preferences we might consider are pretty limited, so I don't expect Platform.Preferences to explode in size and scope.

I think this is a minor addition that doesn't change anything substantial about Platform.Preferences, and as such doesn't need a major discussion since the arguments are basically the same since the introduction of platform preferences. However, if you or others are interested in having a discussion, we can of course do that.

mstr2 · 2024-10-31T20:49:38Z

Changed the naming from reducedScrollBars to persistentScrollBars, as this matches the logic of the OS settings more closely.

kevinrushforth · 2024-11-01T13:44:16Z

My thinking is that a particular preference should be useful and almost universally supported on the desktop platforms to make it into JavaFX API.

This seems like the right criteria to me.

The number of preferences we might consider are pretty limited, so I don't expect Platform.Preferences to explode in size and scope.

Good.

I think this is a minor addition that doesn't change anything substantial about Platform.Preferences, and as such doesn't need a major discussion since the arguments are basically the same since the introduction of platform preferences. However, if you or others are interested in having a discussion, we can of course do that.

With this explanation, I agree.

kevinrushforth · 2024-11-01T13:45:24Z

Reviewers: @andy-goryachev-oracle @lukostyra

(I'll look over the public API and the CSR)

modules/javafx.graphics/src/main/java/javafx/application/Platform.java

andy-goryachev-oracle

the code changes look good, and both the property and the key reflect the platform preference on macOS M1 14.7.

I'll test windows tomorrow, and if someone can test it on linux, that would be fantastic!

andy-goryachev-oracle

looks good on win11, both property and windows key get updated in the monkey tester.

kevinrushforth

Looks good. In addition to reviewing the code and the docs, I tested it on all platforms. On Linux I tested it on Ubuntu 24.04 and on a very old 16.04. There is a minor bug on 16.04 (a warning printed), but it could be done as a follow-up if you prefer.

I reviewed the CSR, so it is ready to finalize.

kevinrushforth · 2024-11-26T20:43:20Z

modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.cpp

@@ -121,6 +121,10 @@ jobject PlatformSupport::collectPreferences() const {
        gboolean enableAnimations = true;
        g_object_get(settings, "gtk-enable-animations", &enableAnimations, NULL);
        putBoolean(env, prefs, "GTK.enable_animations", enableAnimations);
+
+        gboolean overlayScrolling = true;
+        g_object_get(settings, "gtk-overlay-scrolling", &overlayScrolling, NULL);


This causes a warning to be printed during initialization on a very old Ubuntu system (16.04) when running any application:

GLib-GObject-WARNING **: g_object_get_valist: object class 'GtkSettings' has no property named 'gtk-overlay-scrolling'

I see from the docs that this setting was added in GTK 3.24.9. Perhaps a runtime check is in order? It could be done as a follow-up bug since it is just a warning. It correctly reports GTK.overlay_scrolling=true and persistentScrollBars=false.

I've added a runtime check for the gtk-overlay-scrolling property.

kevinrushforth

The warning is gone on my Ubuntu 16.04 system. I note that the GTK.overlay_scrolling preference is not reported on that system (which seems fine) and that the persistentScrollBarsProperty is always false. Everything still works as expected on systems with a newer GTK.

kevinrushforth · 2024-11-27T13:09:23Z

modules/javafx.graphics/src/main/native-glass/gtk/PlatformSupport.cpp

@@ -121,6 +121,12 @@ jobject PlatformSupport::collectPreferences() const {
        gboolean enableAnimations = true;
        g_object_get(settings, "gtk-enable-animations", &enableAnimations, NULL);
        putBoolean(env, prefs, "GTK.enable_animations", enableAnimations);
+
+        if (g_object_class_find_property(G_OBJECT_GET_CLASS(settings), "gtk-overlay-scrolling")) {


Looks good, presuming that we want the property to be absent on systems that don't support this setting.

Yes, a property that isn't available shouldn't be included in the preferences map. We've documented that in Platform.Preferences:

* The preferences that are reported by the platform may be dependent on the operating system version * and its current configuration, so applications should not assume that a particular preference is * always available.

Yes, so this is fine.

andy-goryachev-oracle

ship it!

mstr2 · 2024-12-02T18:04:22Z

/integrate

openjdk · 2024-12-02T18:05:06Z

Going to push as commit da6ad4b.
Since your change was applied there have been 5 commits pushed to the master branch:

03eb8b1: 8344899: Map RT-nnnn bug IDs to JDK-mmmmmmm in JavaFX sources
bdfc338: 8338000: [macos] Remove GlassFullscreenWindow
a45e33c: 8342461: Remove calls to doPrivileged in javafx.web/{android,ios}
f7566ad: 8345063: Centralize version number of the Gradle build file
4af67c1: 8344906: Simplify Java version parsing in the build file

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-02T18:05:14Z

@mstr2 Pushed as commit da6ad4b.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Add reducedScrollBars preference

d37118c

openjdk bot added the rfr Ready for review label Oct 31, 2024

openjdk bot added the csr Need approved CSR to integrate pull request label Oct 31, 2024

Change reducedScrollBars to persistentScrollBars

bd6ecbc

mstr2 changed the title ~~8343336: Add reducedScrollBars preference~~ 8343336: Add persistentScrollBars preference Oct 31, 2024

kevinrushforth reviewed Nov 1, 2024

View reviewed changes

modules/javafx.graphics/src/main/java/javafx/application/Platform.java Show resolved Hide resolved

mstr2 added 2 commits November 1, 2024 18:41

add platform keys to javadoc

00de674

Merge branch 'master' into feature/accessibility-hide-scrollbars

dc0eac5

kevinrushforth self-requested a review November 8, 2024 16:17

andy-goryachev-oracle reviewed Nov 13, 2024

View reviewed changes

andy-goryachev-oracle approved these changes Nov 14, 2024

View reviewed changes

kevinrushforth approved these changes Nov 26, 2024

View reviewed changes

check whether gtk-overlay-scrolling exists

1f194ce

openjdk bot removed the csr Need approved CSR to integrate pull request label Nov 26, 2024

Merge branch 'master' into feature/accessibility-hide-scrollbars

6d44e28

mstr2 requested review from kevinrushforth and andy-goryachev-oracle November 27, 2024 00:23

kevinrushforth approved these changes Nov 27, 2024

View reviewed changes

andy-goryachev-oracle approved these changes Dec 2, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Dec 2, 2024

openjdk bot added the integrated Pull request has been integrated label Dec 2, 2024

openjdk bot closed this Dec 2, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Dec 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8343336: Add persistentScrollBars preference #1618

8343336: Add persistentScrollBars preference #1618

mstr2 commented Oct 31, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Oct 31, 2024

openjdk bot commented Oct 31, 2024 •

edited

Loading

openjdk bot commented Oct 31, 2024

openjdk bot commented Oct 31, 2024

mlbridge bot commented Oct 31, 2024 •

edited

Loading

andy-goryachev-oracle commented Oct 31, 2024

mstr2 commented Oct 31, 2024

mstr2 commented Oct 31, 2024

kevinrushforth commented Nov 1, 2024

kevinrushforth commented Nov 1, 2024

andy-goryachev-oracle left a comment

andy-goryachev-oracle left a comment

kevinrushforth left a comment

kevinrushforth Nov 26, 2024

mstr2 Nov 26, 2024

kevinrushforth left a comment

kevinrushforth Nov 27, 2024

mstr2 Nov 27, 2024

kevinrushforth Nov 27, 2024

andy-goryachev-oracle left a comment

mstr2 commented Dec 2, 2024

openjdk bot commented Dec 2, 2024

openjdk bot commented Dec 2, 2024

8343336: Add persistentScrollBars preference #1618

8343336: Add persistentScrollBars preference #1618

Conversation

mstr2 commented Oct 31, 2024 • edited by openjdk bot Loading

Progress

Issues

Reviewers

Reviewing

bridgekeeper bot commented Oct 31, 2024

openjdk bot commented Oct 31, 2024 • edited Loading

openjdk bot commented Oct 31, 2024

openjdk bot commented Oct 31, 2024

mlbridge bot commented Oct 31, 2024 • edited Loading

Webrevs

andy-goryachev-oracle commented Oct 31, 2024

mstr2 commented Oct 31, 2024

mstr2 commented Oct 31, 2024

kevinrushforth commented Nov 1, 2024

kevinrushforth commented Nov 1, 2024

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Nov 26, 2024

Choose a reason for hiding this comment

mstr2 Nov 26, 2024

Choose a reason for hiding this comment

kevinrushforth left a comment

Choose a reason for hiding this comment

kevinrushforth Nov 27, 2024

Choose a reason for hiding this comment

mstr2 Nov 27, 2024

Choose a reason for hiding this comment

kevinrushforth Nov 27, 2024

Choose a reason for hiding this comment

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

mstr2 commented Dec 2, 2024

openjdk bot commented Dec 2, 2024

openjdk bot commented Dec 2, 2024

mstr2 commented Oct 31, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 31, 2024 •

edited

Loading

mlbridge bot commented Oct 31, 2024 •

edited

Loading